Skip to content

CSHARP-4443: Add comprehensive dictionary LINQ support for all 3 representations #1804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

CSHARP-4443: Add comprehensive dictionary LINQ support for all 3 representations #1804

wants to merge 2 commits into from

Conversation

@adelinowona
Copy link
Contributor

@adelinowona adelinowona commented Oct 31, 2025

No description provided.

@adelinowona adelinowona requested a review from a team as a code owner October 31, 2025 00:48
@adelinowona adelinowona added the feature Adds new user-facing functionality. label Oct 31, 2025
rstam
rstam requested changes Nov 4, 2025
View reviewed changes
Copy link
Contributor

@rstam rstam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I've only reviewed the tests, where I have a lot of suggestions about possible MQL for things you didn't implement or "better" MQL in some cases for things you did implement.

Please take all suggestions with a grain of salt. You may have reasons for thinking that what I think is "better" MQL might not actually be better.

I am postponing reviewing the implementation because if you accept many of the MQL suggestions you will need to make many changes to the implementation.

I will review the implementation after you have accepted/rejected the MQL suggestions.

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp2509Tests.cs

namespace MongoDB.Driver.Tests.Linq.Linq3Implementation.Jira
{
public class CSharp2509Tests : LinqIntegrationTest<CSharp2509Tests.ClassFixture>
Copy link
Contributor

@rstam rstam Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised that these tests were deleted, but Adelin says they duplicate new tests he added in CSharp4443Tests.cs.

We don't usually remove tests... why bother?

Copy link
Contributor

@rstam rstam Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to other reviewers: we have to manually verify that we haven't lost any test coverage as a result of deleting these tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rstam rstam rstam requested changes

@sanych-sun sanych-sun Awaiting requested review from sanych-sun

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

feature Adds new user-facing functionality.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@adelinowona @rstam